Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

nodes/src/nodes/aparavi_aql/aql_schema.py (1)

223-225: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Return a defensive copy of COLUMNS from get_schema_dict().

Line 225 exposes mutable global state by reference, so downstream mutation can corrupt schema output for later calls.

Suggested fix

 def get_schema_dict() -> Dict[str, Any]:
     """Return the structured schema dict for the get_schema tool response."""
-    return {'store': 'STORE', 'columns': COLUMNS}
+    return {'store': 'STORE', 'columns': [col.copy() for col in COLUMNS]}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nodes/src/nodes/aparavi_aql/aql_schema.py` around lines 223 - 225,
get_schema_dict currently returns the global COLUMNS by reference which allows
downstream callers to mutate shared state; update get_schema_dict to return a
defensive copy of COLUMNS (e.g., use copy.deepcopy(COLUMNS) or construct new
dict/list copies) so the returned value cannot modify the module-level COLUMNS;
keep the return shape {'store': 'STORE', 'columns': <copied columns>} and
import/use the copy module if using deepcopy.

nodes/src/nodes/aparavi_aql/aql_client.py (1)

80-93: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard JSON envelope type before dictionary access.

At Line 80, resp.json() can return a non-object JSON value; Line 88 then calls .get(...) and crashes with AttributeError, bypassing your RuntimeError wrapping path.

Suggested fix

             resp.raise_for_status()
             envelope = resp.json()
+            if not isinstance(envelope, dict):
+                raise RuntimeError('Aparavi API returned non-object JSON envelope')
@@
-        data = envelope.get('data') or {}
+        data = envelope.get('data') or {}
+        if not isinstance(data, dict):
+            raise RuntimeError('Aparavi API returned invalid "data" payload')

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nodes/src/nodes/aparavi_aql/aql_client.py` around lines 80 - 93, The code
assumes envelope (from resp.json()) is a dict but resp.json() can return
non-dict values; update the handling in the response parsing (around envelope =
resp.json() and subsequent checks) to validate the type before using dict
methods: after envelope = resp.json(), check isinstance(envelope, dict) and if
not raise a RuntimeError (e.g., "Aparavi API returned non-object JSON envelope")
so the existing envelope.get('status'), envelope.get('message'),
envelope.get('data') and data.get('objects') calls are safe; ensure this
validation sits before the if envelope.get('status') != 'OK' block and before
data = envelope.get('data') or {}.

nodes/src/nodes/aparavi_aql/IInstance.py (2)

146-159: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Include AQL generation failures in the retry path.

At Line 147, _generate_aql(...) runs outside the try, so LLM/generation exceptions bypass retries and return-shape handling.

Suggested fix

         for _ in range(_MAX_AQL_RETRIES):
-            aql = self._generate_aql(question_text, previous_aql=previous_aql, error=last_error)
-
-            # Safety check — block mutation queries
-            if not _aql_safe(aql):
-                return {'error': 'Generated AQL contains unsafe operations', 'aql': aql, 'rows': []}
-
             try:
+                aql = self._generate_aql(question_text, previous_aql=previous_aql, error=last_error)
+
+                # Safety check — block mutation queries
+                if not _aql_safe(aql):
+                    return {'error': 'Generated AQL contains unsafe operations', 'aql': aql, 'rows': []}
+
                 result = client.execute(aql)
                 return {'rows': result['rows'], 'aql': aql, 'count': result['count']}
-            except RuntimeError as exc:
+            except Exception as exc:
                 last_error = str(exc)
-                previous_aql = aql
+                previous_aql = locals().get('aql', previous_aql)

Also applies to: 324-327

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nodes/src/nodes/aparavi_aql/IInstance.py` around lines 146 - 159, The AQL
generation call _generate_aql is currently outside the try so LLM/generation
exceptions escape the retry loop; wrap the call to _generate_aql inside the same
try/except that handles client.execute so generation errors set last_error
(e.g., str(exc)) and previous_aql as appropriate and allow the loop to continue
for up to _MAX_AQL_RETRIES, then after retries return the consistent error shape
{'error': ..., 'aql': aql_or_none, 'rows': []}; apply the same change to the
other occurrence around lines 324-327 (same function/method where _generate_aql
and _aql_safe are used) so both generation and execution exceptions are retried
and produce the same return structure.

---

50-63: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Enforce a strict single-statement SELECT allowlist, not only a mutation blacklist.

At Line 60, non-SELECT/multi-statement queries can still pass if they avoid listed keywords. The gate should require one SELECT statement (optional trailing semicolon) and reject additional statements.

Suggested fix

 _UNSAFE_PATTERN = re.compile(
     r'\b(INSERT|UPDATE|DELETE|DROP|TRUNCATE|ALTER|CREATE|EXEC|EXECUTE)\b',
     re.IGNORECASE,
 )
+_SELECT_START = re.compile(r'^\s*SELECT\b', re.IGNORECASE)
@@
 def _aql_safe(aql: str) -> bool:
-    """Return True if the AQL contains no unsafe mutation keywords."""
-    return not _UNSAFE_PATTERN.search(aql)
+    """Allow only a single SELECT statement with no unsafe tokens."""
+    text = aql.strip()
+    if text.endswith(';'):
+        text = text[:-1].rstrip()
+    if ';' in text:
+        return False
+    if not _SELECT_START.match(text):
+        return False
+    return not _UNSAFE_PATTERN.search(text)

Also applies to: 149-151

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nodes/src/nodes/aparavi_aql/IInstance.py` around lines 50 - 63, The current
_aql_safe function uses _UNSAFE_PATTERN to blacklist mutation keywords but must
instead enforce a strict single-statement SELECT allowlist: update _aql_safe to
validate that the input AQL matches exactly one SELECT statement (optionally
with a single trailing semicolon and whitespace) and rejects anything else
(multiple statements, non-SELECT verbs, or embedded semicolons). Locate the
_aql_safe function and replace the blacklist logic with a regex or parser that
enforces: start-of-string, optional whitespace, the word SELECT, any characters,
optional whitespace, optional single semicolon, optional whitespace,
end-of-string; also update any other usage of _aql_safe (e.g., the other
occurrence referencing the same check) so they use the new strict validation and
not the _UNSAFE_PATTERN blacklist.

nodes/src/nodes/aparavi_aql/aql_schema.py (1)

223-225: ⚠️ Potential issue | 🟡 Minor | 💤 Low value

Shallow copy doesn't protect nested dict mutation.

list(COLUMNS) creates a new list but the dict elements are still shared references. A caller mutating columns[0]['name'] would corrupt global state. Per the previous review suggestion, use a shallow copy of each dict.

Suggested fix

 def get_schema_dict() -> Dict[str, Any]:
     """Return the structured schema dict for the get_schema tool response."""
-    return {'store': 'STORE', 'columns': list(COLUMNS)}
+    return {'store': 'STORE', 'columns': [col.copy() for col in COLUMNS]}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nodes/src/nodes/aparavi_aql/aql_schema.py` around lines 223 - 225,
get_schema_dict currently returns {'store': 'STORE', 'columns': list(COLUMNS)}
which leaves the dict elements in COLUMNS shared; change it to return a new list
of shallow-copied column dicts so callers can't mutate global state (e.g., build
columns as [col.copy() for col in COLUMNS] and return {'store': 'STORE',
'columns': that_list}); update the get_schema_dict function to use this
shallow-copy approach referencing COLUMNS.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

• pnpm-lock.yaml: Language not supported